-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ensure clean-up state after send transaction confirmation #21282
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (16)
|
df44fb6
to
d88e07a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self Review 📑
[utils.i18n :as i18n] | ||
[utils.re-frame :as rf])) | ||
|
||
(defn view | ||
[] | ||
(hot-reload/use-safe-unmount #(rf/dispatch [:wallet/stop-get-suggested-routes])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're configuring the wallet send screen to dispatch the :wallet/stop-get-suggested-routes
event when unmounting the component. Without this, it seems that we'll continue to receive suggested route signals, which can cause some glitches because we're re-processing stale wallet send UI state.
src/utils/money.cljs
Outdated
(try | ||
(.eq ^js bn1 n2) | ||
(catch :default _ false)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're attempting to prevent exceptions from being thrown because of the BigNumber library failing to do a comparison. Perhaps we should not catch all exceptions here, but I think it's safer to assume that any failure would mean the comparison failed, which would mean the two things are not equal.
But maybe there's a better way to handle these exceptions, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanstrom, I think that we can avoid try/catch which is not common in our codebase by checking both arguments with (bignumber?)
before calling .eq
, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what is the best approach, but I've recently updated greater-than
less-than
and greater-than-or-equals
functions with a bignumber?
check. The check was only needed on the first argument to avoid the error. We could use the same approach on equal-to
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@briansztamfater, I also think that everything should be wrapped in checks, including second argument, because for equal-to
second argument causes exception. I wrote an example down below.
Hi, this issue comment was made into a separate issue and assigned to @vkjr @seanstrom your changes look good, if @vkjr has nothing against it we can go forward and use this PR. |
@shivekkhurana thanks I've updated the PR with the link to the issue 👍 @vkjr based on some of the exceptions I saw when debugging, it could be still useful to refactor the code to avoid saving the |
@seanstrom, thanks for the PR, it will stop us from having exception. And I will investigate how that "<0.01" makes its way to that function, it shouldn't happen) |
As @seanstrom correctly assumed, the fail happens because Throwing exception is expected and covered by tests by @ilmotta here, but I think that overall BigNumber is too fragile and we shouldn't allow it to throw exception. Because result of So my suggestion is to wrap all functions in (defn equal-to
[n1 n2]
(when-let [^js bn1 (bignumber n1)]
(when-let [^js bn2 (bignumber n2)]
(.eq ^js bn1 ^js bn2)))) Wdyt, @seanstrom @ilmotta @shivekkhurana ? |
d88e07a
to
a7c0eab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would refactor the code in a way that avoids saving "<0.01" into the database, perhaps we can save an separate keyword like :less-than-something to indicate the same thing. Maybe this would avoid potential number comparison issues in the future.
Thanks a lot for helping with this bug @seanstrom. I was kind of expecting us to fix the root cause to avoid further problems. Even the names to-network-values-for-ui
and from-network-values-for-ui
are good hints that they shouldn't be stored in the app-db. Perhaps not too risky to fix the problem because the values are only used by one event which is dispatched from only one place after a signal arrives. The event also has one unit test, that should help a bit.
But of course, could be a follow-up PR, but would be nice if we never stored formatted data in the app-db, unless there's a good reason, like a performance concern where it's very expensive to recompute in subs and the computed data is read heavy much more than write heavy in the UI.
I fully agree @vkjr. Throwing an exception is quite bad for users because they bubble up in the whole stack. The bignumber library can throw for various reasons, probably not only because the inputs are not instances of bignumbers. It would be a good idea to take a look at the docs or implementation to see the possible causes of errors because we may need to wrap in a The other thing we should do ideally is to have a proper layer (data store ns) that always decodes data from status-go into the proper data types for Clojure. If a string arrives and it's supposed to be a bignum, it shouldn't flow throughout the system for too long as a raw string because eventually it can easily reach code that thinks it's a proper number. I've personally faced this problem multiple times in a previous Clojure project. We extended status-mobile/src/utils/money.cljs Line 65 in 1190fc8
|
91c6356
to
7150e34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More self review after updates 📖
Please have a look when you have a chance 🙏
cc @vkjr @briansztamfater @ilmotta
:from-values-by-chain from-network-values-for-ui | ||
:to-values-by-chain to-network-values-for-ui | ||
:from-values-by-chain from-network-amounts-by-chain | ||
:to-values-by-chain to-network-amounts-by-chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where we were storing the UI strings inside the re-frame state. Now we'll store the original data and do the conversions inside the UI components.
:values network-values | ||
:values (send-utils/network-values-for-ui network-values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one place we were using the UI strings inside network-values, because summary-info will use the string "<0.01" to determine if some UI should be shown. Not sure what the original intent of that code would be, but perhaps that code could be changed too.
(defn ->bignumber | ||
[n] | ||
(if (bignumber? n) n (bignumber n))) | ||
|
||
(defn ->bignumbers | ||
[& nums] | ||
(let [bignums (map ->bignumber nums)] | ||
(when (every? bignumber? bignums) | ||
bignums))) | ||
|
||
(defn greater-than-or-equals | ||
[^js bn1 ^js bn2] | ||
(when (bignumber? bn1) | ||
[^js n1 ^js n2] | ||
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)] | ||
(.greaterThanOrEqualTo bn1 bn2))) | ||
|
||
(defn greater-than | ||
[bn1 bn2] | ||
(when (bignumber? bn1) | ||
[n1 n2] | ||
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)] | ||
(.greaterThan ^js bn1 bn2))) | ||
|
||
(defn less-than | ||
[bn1 bn2] | ||
(when (bignumber? bn1) | ||
[n1 n2] | ||
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)] | ||
(.lessThan ^js bn1 bn2))) | ||
|
||
(defn equal-to | ||
[n1 n2] | ||
(when-let [^js bn1 (bignumber n1)] | ||
(.eq ^js bn1 n2))) | ||
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)] | ||
(.eq ^js bn1 bn2))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're adding some additional checks for big number instances when using our big number utility functions. Let me what you think 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this approach!
71% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
|
@seanstrom, thanks for the improvements! 🙏 |
…irmed the transaction
…tate during render
7150e34
to
27cbe39
Compare
partially fixes #21251
Summary
"<0.01"
"<0.01"
for some wallet ui state, but that state can be accidentally used with BigNumber calculations, which can exceptions.status-mobile/src/status_im/contexts/wallet/send/utils.cljs
Line 67 in 6966432
status-mobile/src/quo/components/wallet/summary_info/view.cljs
Lines 33 to 40 in 2f3d3fc
"<0.01"
which would cause things to throw."<0.01"
into the database, perhaps we can save an separate keyword like:less-than-something
to indicate the same thing. Maybe this would avoid potential number comparison issues in the future.Testing notes
Platforms
Areas that maybe impacted
Functional
Steps to test
0.01
, like0.001
Before and after screenshots comparison
Before Changes
Screen.Recording.2024-09-19.at.08.11.47.mov
After Changes
Screen.Recording.2024-09-19.at.08.10.34.mov
status: ready